Conversation
|
@copilot can you create a new PR based on this one to update the test cases matching the code changes? |
| return diff.seconds | ||
|
|
||
|
|
||
| class VersionAutomationRule(PolymorphicModel, TimeStampedModel): |
There was a problem hiding this comment.
I will delete this model in a following PR and migration. We are not using it anymore.
stsewd
left a comment
stsewd
left a comment
There was a problem hiding this comment.
Looks like this is missing moving the models and forms to the projects app.
| rule_triggered_for_project = True | ||
| rule_triggered_for_version = True | ||
| rule.run(version) | ||
|
|
||
| # We only trigger the first matching rule, | ||
| # We only trigger the first matching rule per version | ||
| # to avoid triggering multiple builds for the same tag/branches. | ||
| break | ||
| if rule_triggered_for_version: | ||
| break |
There was a problem hiding this comment.
There is no need to track this in two variables. Since you only want to know if at least one rule matched, you can use a single variable.
| rule_triggered_for_project = True | |
| rule_triggered_for_version = True | |
| rule.run(version) | |
| # We only trigger the first matching rule, | |
| # We only trigger the first matching rule per version | |
| # to avoid triggering multiple builds for the same tag/branches. | |
| break | |
| if rule_triggered_for_version: | |
| break | |
| rule_triggered = True | |
| rule.run(version) | |
| # We only trigger the first matching rule per version | |
| # to avoid triggering multiple builds for the same tag/branches. | |
| break |
There was a problem hiding this comment.
We need to check all the rules for all the versions. Using only one variable, if the first rule is triggered on the first version, we will be skipping the other versions.
This reverts commit 19b5ada.
agjohnson
left a comment
agjohnson
left a comment
There was a problem hiding this comment.
There's some needed on the UI side but overall this looks good.
| commit_message = self._get_commit_message_from_pull_request_event(project) | ||
| labels = self._get_labels_from_pull_request_event(project) |
There was a problem hiding this comment.
This should be outside the loop, it's the same for all projects.
There was a problem hiding this comment.
👍🏼 -- hrm, we use the project.remote_repository.remote_id to get the GHApp installation inside _get_commit_message_from_pull_request_event. I think we need that to be specific for each project.
| ) | ||
| return False | ||
|
|
||
| def match_webhook(self, changed_files=None, commit_message=None, labels=None): |
There was a problem hiding this comment.
having two methods to check a version moves the responsibility of calling both to the caller, looks like this is already missing one place in api/v2/utils.py (run_version_automation_rules).
There was a problem hiding this comment.
That is on purpose. Do we want to apply the webhook filters when running rules that apply over versions only? Like activate/deactivate a version; make it as default, etc?
This is the same we are using to select projects under the Teams admin page.
…mitos/webhook-filters-v2
This is the initial proposal to migrate our current
VersionAutomationRulesandRegexAutomationRulesto only one model calledAutomationRuleas discussed.I created a basic UI just to be able to use the feature; but we need to work on ext-theme for that. I think it won't be a lot of work, but we need:
version_typesshould be a multi-select fieldversion_match_patternshould be shown/hidden based on another input valuev2#12788